feat: add crawling provider#52
Conversation
nirolevy
force-pushed
the
crawling-provider
branch
4 times, most recently
from
17d6f58 to
6f5f44b
Compare
nirolevy
force-pushed
the
crawling-provider
branch
2 times, most recently
from
b7ce5be to
7c4a2b4
Compare
nirolevy
force-pushed
the
crawling-provider
branch
from
7c4a2b4 to
b11b250
Compare
asafMasa
left a comment
asafMasa
left a comment
There was a problem hiding this comment.
🥇 Good job, but I have some remarks, thanks.
| @withSpanAsyncV4 | ||
| public async getFile(filePath: string): Promise<Buffer> { | ||
| const logContext = { ...this.logContext, function: this.getFile.name }; | ||
| const pvPath = this.config.pvPath; |
There was a problem hiding this comment.
This can be set once in the Ctor instead of being declared every time this method is called (or you can use 'this.config.pvPath' instead)
| } | ||
|
|
||
| public async createFileOfModel(model: string, file: string): Promise<void> { | ||
| public async createFileOfModel(model: string, file: string): Promise<Buffer> { |
There was a problem hiding this comment.
Instead of creating fake content here and returning it, consider passing an optional argument of data to be written, and keep the method return type as Promise
| }); | ||
|
|
||
| describe('getFile', () => { | ||
| it('When calling getFile, should get the file content from pv path', async () => { |
There was a problem hiding this comment.
Why didn't you add the test of "it(When the file does not exist in the storage, throws error", same as in the S3 provider? Is there a difference in the expected response from the "getFile" in the provider?
| @@ -0,0 +1,41 @@ | |||
| import config from 'config'; | |||
There was a problem hiding this comment.
🥇 Thanks for adding the provider tests
| }, | ||
| "crawling": { | ||
| "extension": ".json", | ||
| "nestedJsonPath": "$.root..uri", |
There was a problem hiding this comment.
Please add support for both "uri" and "url" (old legacy 3Dtiles models)
| @@ -0,0 +1,137 @@ | |||
| import fs from 'fs'; | |||
| import os from 'os'; | |||
There was a problem hiding this comment.
Please use from 'node:os';
| modelId, | ||
| modelName, | ||
| }); | ||
| throw new AppError(StatusCodes.NOT_ACCEPTABLE, 'File is not a valid JSON', false); |
There was a problem hiding this comment.
Have you added this status to the valid openAPI status responses?
|
|
||
| describe('Bad Path', function () { | ||
| // All requests with status code of 400 | ||
| it('should return 400 status code if a job with same name exists in job manager', async function () { |
There was a problem hiding this comment.
Add test with an invalid file type returns 406 NOT_ACCEPTABLE response
| }); | ||
| throw new AppError(StatusCodes.NOT_ACCEPTABLE, 'File is not a valid JSON', false); | ||
| } else { | ||
| throw err; |
There was a problem hiding this comment.
Is this exception logged?
|
|
||
| @injectable() | ||
| export class CrawlingProvider implements Provider { | ||
| private readonly logContext: LogContext; |
There was a problem hiding this comment.
I understand that you added another "Provider" for the crawling, but I think the approach should have been to create an abstractBaseProvider for NFS and S3 to inherit from, as we don't want to keep the old behavior.
We want to change it.
2 participants
Further information:
Add provider to traverse JSON tilesets to map the FS